(consistency) Let's start all tests with test.

We are already not doing that in the codebase. But if we are to come up with a rule, maybe, given that "test" in the name is noise that makes names longer, we consider the style where "test" is never specified?

currently multiple testing method naming conventions have co-existed in the repo already. I didn't object to dropping "test" from method names in @stIncMale 's testing code.

but I agree, in this PR, there is no reason not to change xxxTest to testxxx. Updated.

(consistency) Acronyms should use init-upper case, as for example you did with SqlAstTranslatorFactory

changed to testComparisonByLte, together with other similar changes.

(duplication) Each of these tests begins with scope.inTransaction(session -> assertContactQueryResult(. Can this logic moved into before and after?

it is assertion related logic so I think it should not be moved to life cycle methods.

guess you are referring to stuff other than assertion. Yeah, we could move transaction and session creation and closing to class fields and lifecycle methods, but those are too lower-level, error-prone and is distracting. That is why we use hibernate-testing library to simplify. The following statement:

scope.inTransaction(session -> ...);

will abstract away all these low-level stuff (they are tedious and error-prone) so we can focus on our business logic.
Not sure whether I understand your point correctly.

It's notable whenever we have to add a new field to this class in order to share state as we visit the HQL AST, as it creates opportunities for that state to get out of sync with reality. In this case, for example, I see code which adds to the set, and code that queries for a predicate in the set, but no code that removes anything from the set. I'm not sure, but it seems like there is a bug lurking in here somewhere.

We don't need to remove it for such translator is one-time usage stuff. It is dynamically created for each translation input and once the translation is done, it is subject to garbage collecting. The transalation factory is singleton global object, but the various translators are purely dynamic stuff. It is perfectly as expected to store ad-hoc data sharable among various visitor methods without considering thread safety issue, for the translator is meant to be run sequentially.

If we want to help a little bit on garbage collecting, maybe we can do something like in Hibernate's clearup method below:

	protected void cleanup() {
		if ( lazySessionWrapperOptions != null ) {
			lazySessionWrapperOptions.cleanup();
			lazySessionWrapperOptions = null;
		}
		this.jdbcParameterBindings = null;
		this.lockOptions = null;
		this.limit = null;
		setOffsetParameter( null );
		setLimitParameter( null );
	}

so we could set the set to null, but do we need to do that? Seems early optimization for now?

Another similar doubt is Hibernate often adopts lazy-initialization to save perf. So the pattern would be:

private Set<Predicate> subPredicatesInNegatedGroupedPredicate;

// create it first when it is used and it is null

I didn't do that for the same reason. Overkill?

My concern is not at all related to performance, but rather correctness. Is it possible that with a more complicated query the subPredicatesInNegatedGroupedPredicate field will need to be used more than once to pass information up the stack? If so, it seems like it would need to be cleared after the value is transmitted, to prepare it to be used once again. We were very careful with that in our use of astVisitorValueHolder, so I wonder if some of the same concerns apply to this field.

I don't think we have such concern. Regardless of how complicated the query is, we have only one query as the translation process input, right? This set's scope will be bound to the translation process so bound to the query. Let us say the query contains multple levels of embedded subqueries (the most complicated query I could think of) and each subquery has its own negated grouped predicate. During the translation, each different not (...) will end up with different SQL AST nodes, so there is no conflict here from my understanding.

It is different from astVisitorValueHolder for it is a collection data structure, so different SQL AST nodes won't conflict and they simply are inserted and queried based on their SQL AST node references. astVisitorValueHolder is super special and it is tightly coupled with stack to work around method returning void restriction; but this is a common collection data structure which simply collect info during the translation process. it is very similar to the following query parameter bindering collector field:

private final List<JdbcParameterBinder> parameterBinders = new ArrayList<>();

Regardless of where the parameter lies, the above will collect them. subPredicatesInNegatedGroupedPredicate serves similar role. I did try to make it work within astVisitorValueHolder but it seems hard and unnecessary.

Yeah, if I follow the spirit of astVisitorValueHolder, I should make the folloiwng changes:

1. change the field type to Predicate currentSubPredicateInNegatedGroupedPredicate
2. carefully maintain its value including setting its value to null when it was used (there could be possibility that it is never used if the predicate is not supported and FeatureNotSupportedException is thrown)

but it seems unnecessarily complicated, so I simply use a collection data structure to make it simple and easy to understand.

Finally I figured out that we don't need this set at all. The reason is we should translate the following visitor method directly:

    @Override
    public void visitNegatedPredicate(NegatedPredicate negatedPredicate) {
        var filter = acceptAndYield(negatedPredicate.getPredicate(), FILTER);
        if (filter instanceof AstFieldOperationFilter fieldOperationFilter) {
            astVisitorValueHolder.yield(FILTER, fieldOperationFilter.negated());
        } else {
            astVisitorValueHolder.yield(FILTER, new AstNorFilter(singletonList(filter)));
        }
    }

Here we use two kinds of filters:

1. AstFieldOperationFilter: $not operator could only be used for this kind of filter;
2. AstNorFilter: $nor is a generic filter applying on any expression or expression list (we only use it for an expression for there is no counterpart of expression list in Hibernate).

In theory, it seems we could use $nor for both cases; the choice to use AstFieldOperationFilter in case 1 is to add the support of $not field filter operation.

The following testing case is added to showcase why $nor is required in case 2 above:

@Test
    void testNorFilterOperation(SessionFactoryScope scope) {
        scope.inTransaction(session -> assertContactQueryResult(
                session, "from Contact where not (country = 'CANADA' or age <= 18)", null, List.of(5)));
    }

For such ad-hoc predicate negation, $not won't work for it could only used for one field filter operation. Seems $nor is the only way to translate it.

@vbabanin I'm going to leave this open for you to consider, but please feel free to resolve if you are satisified.

We did not start handling jdbcParameterBindings here, but we removed the check. Why is this correct, was the check placement was wrong originally?

We start using it a lot in this PR, as follows:

@Test
    void testProjectOperation(SessionFactoryScope scope) {
        scope.inTransaction(session -> {
            var results = session.createSelectionQuery(
                            "select name, age from Contact where country = :country", Object[].class)
                    .setParameter("country", Country.CANADA.name())
                    .getResultList();
            assertThat(results)
                    .containsExactly(new Object[] {"Mary", 35}, new Object[] {"Dylan", 7}, new Object[] {"Lucy", 78});
        });
    }

the setParameter() will end up with non-null jdbcParametersBinding.

So we now have examples that result in non-null jdbcParameterBindings being passed to SelectMqlTranslator.translate. But we still don't handle the jdbcParameterBindings parameter. Under what conditions is it OK to do this, and why? It probably can't be that it's OK to always ignore jdbcParameterBindings, because then the parameter should not have existed.

Good question. After digging into the default Hibernate SQL translator I figured out its usage.
It seems jdbcParameterBindings is mainly used after translation is done; there is only one edge case that it is required during translation. That is when the parameter rendering mode is of INLINE. The code is as below:

	@Override
	public void visitParameter(JdbcParameter jdbcParameter) {
		switch ( getParameterRenderingMode() ) {
			case NO_UNTYPED:
			case NO_PLAIN_PARAMETER: {
				renderCasted( jdbcParameter );
				break;
			}
			case INLINE_PARAMETERS:
			case INLINE_ALL_PARAMETERS: {
				renderExpressionAsLiteral( jdbcParameter, jdbcParameterBindings );
				break;
			}
			case WRAP_ALL_PARAMETERS:
				renderWrappedParameter( jdbcParameter );
				break;
			case DEFAULT:
			default: {
				visitParameterAsParameter( jdbcParameter );
				break;
			}
		}
	}

so jdbcParameterBindings will be used to fetch all the binded parameter values to achieve the INLINE goal. Why? I have no further idea, but it seems irrelevant to our product.

I am not sure that's a correct assessment:

1. The AbstractSqlAstTranslator.visitParameter method uses jdbcParameterBindings by only passing it to the renderExpressionAsLiteral method, where AbstractSqlAstTranslator.jdbcParameterBindings is effectively ignored (there is a null check for this argument, but that's it).
2. jdbcParameterBindings is, however, actually used in the following methods:
   2.1 AbstractSqlAstTranslator.getParameterBindValue
   2.2 AbstractSqlAstTranslator.renderFetchPlusOffsetExpressionAsSingleParameter
   2.3 AbstractSqlAstTranslator.OffsetReceivingParameterBinder.bindParameterValue

But I can't say anything further. If we fail to understand what jdbcParameterBindings may be used for, let's document this parameter in SelectMqlTranslator.translate to reflect that we don't know what it is and when we are to use it, so we are ignoring it for now, as sometimes it is not null and jdbcParameterBindings.getBindings() is not empty.

I didn't explain its full usage for I think it is distracting for now and I mainly focused on its main usage (from my perspective which is different than yours). Of course, parameter INLINE is not our concern. The other main usages include:

• returning it intact in AbstractSqlAstTranslator.getParameterBindValue
• Limit processing but only as assertion purpose, not used directly.

We have a ticket for the second concern (https://jira.mongodb.org/browse/HIBERNATE-70). SQL servers have dramatically different syntaxes when it comes to Limit feature (offset, limit), so a separate processing is required. When Limit is passed through queryOptions, AbstractSqlAstTranslator needs to create two JdbcParameters maximally and populate them into addtionalJdbcParameter; when additionalJdbcParameter needs to be populated, jdbcParameterBindings needs to be non-null as Hibernate's internal assertion logic. But that is the only usage of jdbcParameterBindings, for it is always used in read-only mode in translator; additionaol jdbc parameters should be stored elsewhere.

When jdbcParameterBindings is non-null? When explicit HQL is used. It could be null even for our previous loading by primary key scenario; but when explicit HQL is used, it should be non-null.

Overall, I think we can be certain we don't need to worry about its usage in this PR

When Limit is passed through queryOptions, AbstractSqlAstTranslator needs to create two JdbcParameters maximally and populate them into addtionalJdbcParameter

What is addtionalJdbcParameter that you are talking about? It has neither been introduced into the discussion, nor can I find it anywhere in either our codebase (https://github.com/mongodb/mongo-hibernate, including the PR at hand), or the Hibernate ORM codebase (https://github.com/hibernate/hibernate-orm).

Sorry, I messed up. There is no additionalJdbcParameters. When two additional JdbcParameters are created by QueryOptions, they will end up with field variables in AbstractSqlAstTranslator in Hibernate ORM.

I studied further and I agree my original INLINE claim is missing the point. I think now I could explain the usage of jdbcParameterBindings in select query translation a little bit more confidently:

Ultimately our translation goal is to provide a org.hibernate.sql.exec.spi.JdbcOperationQuerySelect object so Hibernate will know of how to go about JDBC processing. The class (https://github.com/hibernate/hibernate-orm/blob/main/hibernate-core/src/main/java/org/hibernate/sql/exec/spi/JdbcOperationQuerySelect.java#L50) has sql as a constructor parameter, but it also has many additional data we need to provide:

public JdbcOperationQuerySelect(
			String sql,
			List<JdbcParameterBinder> parameterBinders,
			JdbcValuesMappingProducer jdbcValuesMappingProducer,
			Set<String> affectedTableNames,
			int rowsToSkip,
			int maxRows,
			Map<JdbcParameter, JdbcParameterBinding> appliedParameters,
			JdbcLockStrategy jdbcLockStrategy,
			JdbcParameter offsetParameter,
			JdbcParameter limitParameter) {

The last two JdbcParameter (offsetParameter and limitParameter) objects are created from QueryOptions alone, but when it comes to the values of rowsToSkip and maxRows, they could come either from QueryOptions or in HQL per se (the former takes precedence over the latter). When they come from HQL, the jdbcParameterBindings would be relied upon to look up the value. That is one of the main usages of jdbcParameterBindings, from my current understanding.

Below are the two methods to compute the rowsToSkip and maxRows:

protected int getRowsToSkip(SelectStatement sqlAstSelect, JdbcParameterBindings jdbcParameterBindings) {
		if ( hasLimit() ) {
			if ( offsetParameter != null && needsRowsToSkip() ) {
				return interpretExpression( offsetParameter, jdbcParameterBindings );
			}
		}
		else {
			final Expression offsetClauseExpression = sqlAstSelect.getQueryPart().getOffsetClauseExpression();
			if ( offsetClauseExpression != null && needsRowsToSkip() ) {
				return interpretExpression( offsetClauseExpression, jdbcParameterBindings );
			}
		}
		return 0;
	}

	protected int getMaxRows(SelectStatement sqlAstSelect, JdbcParameterBindings jdbcParameterBindings, int rowsToSkip) {
		if ( hasLimit() ) {
			if ( limitParameter != null && needsMaxRows() ) {
				final Number fetchCount = interpretExpression( limitParameter, jdbcParameterBindings );
				return rowsToSkip + fetchCount.intValue();
			}
		}
		else {
			final Expression fetchClauseExpression = sqlAstSelect.getQueryPart().getFetchClauseExpression();
			if ( fetchClauseExpression != null && needsMaxRows() ) {
				final Number fetchCount = interpretExpression( fetchClauseExpression, jdbcParameterBindings );
				return rowsToSkip + fetchCount.intValue();
			}
		}
		return Integer.MAX_VALUE;
	}

I think we could figure out all the details in our future ticket and I would not pretend I know of all the details for now. But seems the jdbcParameterBindings is still used in read-only mode.

@vbabanin I'm going to leave this open for you to consider, but please feel free to resolve if you are satisified.

We should test for the produced MQL. To do this, we can:

• Create a package-private listener = null, in MongoStatement (and any other place we send mql), which, if not-null, will be called with the MQL string (on the line before that string is sent to the Java driver). An implementation of this listener would be created by tests, and would collect all such MQL into a list. The list would be cleared at the start of tests, and checked by the above assertion, that the single entry there equals what is above.
• The whereMql could parse and wrap the string, to avoid having to repeat the aggregation boilerplate across all tests.
• The generated MQL would be compared using: assertEquals(expectedValue, actualValue, actualValue.toString().replace("\"", "'"));, where the values are both BsonValue (this is to allow readable, multiline formatting of the expected mql string in tests (where needed).
• The person1 is a Contact, and the assert performs a full comparison.

Let's only change this one test, to see what this looks like, before modifying all tests.

I think this would provide all the coverage of AstAndFilterTests, and more, so it would also eliminate the "weaker" unit tests.

Hibernate has its own statement listener, but with the limit that it retains parameter. Is that enough?

Did as you suggested.

I changed all the testing methods after implementing a CommandListener from scratch (putting it into a global package so it could be reused in the future for other tickets).

These tests seem to be the same as the tests above. That is, they could have been written as:

        sessionFactoryScope.inTransaction(session -> assertContactQueryResult(
                session,
                "from Item where booleanField = true"),
                q -> {},
                List.of(N)));

Is there any reason they are written differently in this class? It also seems they could all just be in the same file.

(If there is no reason, let's avoid making any changes until the other comment regarding testing against mql is resolved.)

no special reason. I created a separate testing class so we could focus on query literal exclusively, then I followed the same pattern focusing on the query literal concern alone.

I've merged two testing classes into one

Should we be testing parameterizations for all of these?

I think that we should use the same entity for everything, so that it is familiar and we do not have to think of something different when opening another file. Let's use semantic names, like you did in the previous tests I reviewed. I liked those - for me, it made it surprisingly easier to understand what a query was doing when the parameters were realistic. I suggest the following:

String title;
String author;
Integer year;
Long isbn13;
Double rating;
BigDecimal price;

This testing class or testing cases are focusing on one thing: query literal, so parameterizations is not in the scope.

The semantic naming is fine, but remember the key part here is the literal data type so using field name containing types makes the HQL self-explanatory.

For normal entity classes (like in our previous CRUD testing cases), I would choose semantic field names. Could we retain the current field names for I do think they are better to reflect our testing purpose?

I did as you suggested. I think I can use literal suffix to make the literal testing self-explanatory.

visitFieldPath name suggests we're traversing an expression tree to compute some boolean of unclear purpose. In reality, we're simply checking whether this is a field. Something like, for example, isFieldExpression might better reflect its intent. What do you think?

I think it makes perfect sense. Changed as suggested.

Currently, FROM Contact WHERE age = null is translated to { "age": { "$eq": null } }, which differs from what a user would expect using HQL/SQL - as such a condition would omit results. Is this expected behaviour?

I didn’t find a test covering this scenario, so I assume handling null comparisons may be out of scope for this PR. if yes, should we throw an unsupported exception for now, with a reference to a follow-up JIRA ticket?

There's also the case to consider when a parameter is used, but it's bound to null.

Yeah, null or missing field is out of scope of this PR and a separate ticket was created for that purpose: https://jira.mongodb.org/browse/HIBERNATE-74

I added logic to throw exception when null literal value is used.

As Jeff reminded, we should add similar safety net against parameter usage with null value. I added a new checkJdbcParameterBindings method to throw exception when any of the parameters is using null value.

951e64d

Is there a way to intercept the final query sent to the database (with filled-in parameters)? That might be more suitable for an integration test and help verify that the query was constructed as expected.

I suspect it could be done by configuring the MongoClientSettings with a CommandListener, similar to what we do in driver unified tests.

Hibernate doesn't expose the complete SQL (not sure about the reason though). Yeah, we could tap into the extension point we implemented to customize MongoClientSettings by adding a CommandListener.

I implemented using our extension mechanism by creating a TestCommandListener (put into a generic package so it could be reused in the future). Let me know whether my understanding of CommandListener is incorrect or partially correct.

a821ec3